-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new analyzer for ICorsHeaderAppender/CorsHeaderAppender usage #326
Add new analyzer for ICorsHeaderAppender/CorsHeaderAppender usage #326
Conversation
…rAppender/CorsHeaderAppender
/// <summary> | ||
/// We never want to allow manual instantiation of this object, so no whitelist here. | ||
/// </summary> | ||
private static void PreventManualInstantiation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is overkill. I figured for safety's sake I would include it, but if it's not worth it I can strip it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
} | ||
|
||
foreach( var parameter in constructor.ParameterList.Parameters ) { | ||
INamedTypeSymbol baseType = context.SemanticModel.GetTypeInfo( parameter.Type ).Type as INamedTypeSymbol; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseType
is a bit confusing (because of base classes) maybe paramType
?
return; | ||
} | ||
|
||
var parentClasses = context.Node.Ancestors().Where( a => a.IsKind( SyntaxKind.ClassDeclaration ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I hadn't considered that case. This makes sense to me (permissions applying to children.)
|
||
INamedTypeSymbol baseType = context.SemanticModel.GetTypeInfo( instantiation ).Type as INamedTypeSymbol; | ||
|
||
if( baseType.IsNullOrErrorType() || !baseType.Equals( classType ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd switch this to baseType == null
. It can't be the error type because it got casted to INamedTypeSymbol
. In generally I'm not a fan of that extension method (it's defined in this lib) because it's very imprecise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, fair enough. Will do!
"D2L.LP.Web.Files.FileViewing.Default.StreamFileViewerResultFactory" | ||
}.ToImmutableHashSet(); | ||
|
||
private static bool IsClassWhitelisted( string className ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be slightly better to take ITypeSymbol
(ISymbol
?) instead of string
- minorly reduces the amount of string
usage (and saves the callers doing .ToString()
anyway.)
nit: I try to keep the repo to 80 columns so diffs render good in GitHub. Don't worry about it for this PR; I'll look at setting up editorconfig today so we can just make your VS do it automatically. |
This looks great! I like starting simple - there's definitely room to grow this one into a more general tool for visibility control. |
I left some comments asking for some minor changes but looks good! |
…Symbol.IsNullOrError checks with simple null checks
Some of the 80 column width issues were fairly easy to correct, so I just took care of them while I was in there. |
|
||
private static bool IsClassWhitelisted( ISymbol classSymbol ) { | ||
return WhitelistedClasses.Contains( classSymbol.ToString() ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@j3parker, I thought this was what you were getting at here, but if I've misunderstood, let me know and I'll change this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
…rs.Visibility namespace (for ease of future generalization)
Note that the underlying interface and class referenced here (ICorsHeaderAppender and CorsHeaderAppender) aren't implemented yet. I need to wait on some dependent work to make sure the implementation actually works but, I figure it can't hurt to have the analyzer here first.
I did try to use other analyzers as a model for how to structure this stuff, but apologies in advance if there's anything I missed or didn't implement ideally. :)